Get instance id for desired control-queue(s) by pasaini-microsoft · Pull Request #1069 · Azure/durabletask

pasaini-microsoft · 2024-04-19T20:35:38Z

Motivation

#1079

Issue: No way of targeting an orchestrator instance to a desired control-queue.

We have been facing issues where DTF orchestration used to get stuck at random. Given that customer load is not very regular in our service, it was challenging to understand upfront if the orchestration would be processed or will be stuck.
More often customers used to reach out with incidents complaining their request not completing for long time.
This is where we needed orchestration instances to observe health of each queue by targeting one instance for desired control-queue.

Motivation:

motivation was to reduce the TTD for finding if orchestration can be stuck/waiting-forever in a control-queue irrespective of the cause.

Issue: No way to load lightly loaded control-queues.

We have face a few situations where some of control-queues are overwhelmed with orchestration instances while the others are happily processing almost nothing.

Motivation:

motivation was to target new instances of orchestration instances to set control-queue which are not heavily loaded

Proposal

API to generate instance id for a set of control-queues.

This API receives set of control-queues and prefix for instance id.
Implementation detail is: Allow special way of creating instance id with a suffix unsigned integer after delimiter '!' and explicitly use that value to route to control-queue (say suffixNumber % partitionCount). If this pattern is not used, it would goes back to default (current) which is hash(instance-id)%partition-count.

davidmrdavid

Some thoughts

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

…ettings.cs Co-authored-by: David Justo <david.justo.1996@gmail.com>

davidmrdavid · 2024-07-10T20:27:33Z

Test/DurableTask.AzureStorage.Tests/TestPartitionIndex.cs

+            controlQueueNumberToNameMap = new Dictionary<string, int>();
+
+            for (int i = 0; i < partitionCount; i++)
+            {
+                var controlQueueName = AzureStorageOrchestrationService.GetControlQueueName(settings.TaskHubName, i);
+                controlQueueNumberToNameMap[controlQueueName] = i;
+            }
+        }


are we still using this in the new tests? No, right?

davidmrdavid

Have we tested this for external events as well?

…https://github.com/pasaini-microsoft/durabletask into users/pasaini/main/InstanceIdForSelectedControlQueue

davidmrdavid · 2024-08-20T16:57:11Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs

+        /// <summary>
+        /// Whether to allow instanceIDs to use special syntax to land on a specific partition.
+        /// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.
+        /// </summary>
+        public bool EnableExplicitPartitionPlacement { get; set; } = false;


Something to consider - it is not safe to change this from false to true (or vice-versa) while an orchestrator with the special syntax is in-flight. If we do that, any pre-existing messages for that orchestrator may now be considered to be "in the wrong queue".

Let's call this out in the intellisense

davidmrdavid · 2024-08-20T17:04:06Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+
+            int placementSeparatorPosition = instanceId.LastIndexOf('!');
+
+            // if the instance id ends with !nnn, where nnn is an unsigned number, it indicates explicit partition placement


can we add a test that documents the behavior if the customer uses an instanceID with multiple ! in there? Say instanceID "A!1!B!3` should probably map to partition "3", right?

Let's also add a test that checks that instanceID myinstanceID!NotANumber does not trigger any errors / that it correctly ignores the explicit placement logic.

Thank you for adding the first test, I think we're just missing the very last one:

Let's also add a test that checks that instanceID myinstanceID!NotANumber does not trigger any errors / that it correctly ignores the explicit placement logic.

davidmrdavid

This looks almost ready to me, but please note there's some outstanding suggestions (and a missing test request from my last review).

FYI @gillum, whose approval we should seek before merging this.

davidmrdavid · 2024-12-02T17:53:41Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+
+            int placementSeparatorPosition = instanceId.LastIndexOf('!');
+
+            // if the instance id ends with !nnn, where nnn is an unsigned number, it indicates explicit partition placement


Thank you for adding the first test, I think we're just missing the very last one:

Let's also add a test that checks that instanceID myinstanceID!NotANumber does not trigger any errors / that it correctly ignores the explicit placement logic.

davidmrdavid · 2024-12-02T18:13:08Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs

+        /// <summary>
+        /// Whether to allow instanceIDs to use special syntax to land on a specific partition.
+        /// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.
+        /// ** DO NOT CHANGE THIS FLAG FOR PRE-EXISTING MESSAGES AS IT MAY BE CONSIDERED IN THE WRONG QUEUE **
+        /// </summary>


nit - I believe remark is the right docs tag for this sort of thing. Let's also not use all-caps for comments, I realize it has a useful effect, but it's not in our usual style for customer-facing docs.

Suggested change

/// <summary>

/// Whether to allow instanceIDs to use special syntax to land on a specific partition.

/// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.

/// ** DO NOT CHANGE THIS FLAG FOR PRE-EXISTING MESSAGES AS IT MAY BE CONSIDERED IN THE WRONG QUEUE **

/// </summary>

/// <summary>

/// Whether to allow instanceIDs to use special syntax to land on a specific partition.

/// If enabled, when an instanceID ends with suffix '!nnn', where 'nnn' is an unsigned number, the instance will land on the partition/queue for to that number.

/// ** DO NOT CHANGE THIS FLAG FOR PRE-EXISTING MESSAGES AS IT MAY BE CONSIDERED IN THE WRONG QUEUE **

/// </summary>

/// <remarks>

/// It is not generally safe to change to this flag for pre-existing TaskHubs, as it may change the expected target queue for an instanceID.

/// </remarks>

davidmrdavid

FYI - I'll take the liberty of deleting the deleting the comment about adding a test at the beginning of TaskHubClient, since this is public documentation so it does not belong there. After that, I'll leave a 'LGTM'

src/DurableTask.Core/TaskHubClient.cs

davidmrdavid · 2024-12-02T18:25:28Z

src/DurableTask.Core/TaskHubClient.cs

        }

        /// <summary>
+        /// Add test for this.


Suggested change

/// Add test for this.

davidmrdavid

LGTM. Please get an approval from a current repo owner (@cgillum, or @jviau, for example) before merging. But this seems good to me, and that any remaining tests we could just add on your behalf with ease.

pasaini-microsoft added 3 commits April 19, 2024 12:42

Get instance id for desired control-queue(s)

409edba

Bringing randomization to instance ids.

2280605

Updating logic for consistenc with netherite.

e98bc4d

davidmrdavid reviewed Jul 8, 2024

View reviewed changes

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs Outdated Show resolved Hide resolved

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs Show resolved Hide resolved

pasaini-microsoft and others added 2 commits July 9, 2024 21:21

Updating tests

e9ee9c0

Update src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceS…

12719fa

…ettings.cs Co-authored-by: David Justo <david.justo.1996@gmail.com>

davidmrdavid reviewed Jul 10, 2024

View reviewed changes

pasaini-microsoft added 2 commits July 29, 2024 06:41

Addressing PR comments.

a07015d

Merge branch 'users/pasaini/main/InstanceIdForSelectedControlQueue' of …

f39724d

…https://github.com/pasaini-microsoft/durabletask into users/pasaini/main/InstanceIdForSelectedControlQueue

davidmrdavid reviewed Aug 20, 2024

View reviewed changes

Addressed comments.

cb9adbc

davidmrdavid reviewed Dec 2, 2024

View reviewed changes

pasaini-microsoft added 2 commits December 2, 2024 10:23

Addressing PR comments.

7c6811d

Addressing PR comments.

be2f775

davidmrdavid reviewed Dec 2, 2024

View reviewed changes

Update src/DurableTask.Core/TaskHubClient.cs

0284c46

davidmrdavid reviewed Dec 2, 2024

View reviewed changes


		int placementSeparatorPosition = instanceId.LastIndexOf('!');

		// if the instance id ends with !nnn, where nnn is an unsigned number, it indicates explicit partition placement

Conversation

pasaini-microsoft commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Issue: No way of targeting an orchestrator instance to a desired control-queue.

Motivation:

Issue: No way to load lightly loaded control-queues.

Motivation:

Proposal

API to generate instance id for a set of control-queues.

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pasaini-microsoft commented Apr 19, 2024 •

edited

Loading